Move more model classes to records #108
Conversation
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #108 +/- ##
==========================================
- Coverage 63.76% 63.75% -0.02%
==========================================
Files 43 43
Lines 723 720 -3
Branches 58 57 -1
==========================================
- Hits 461 459 -2
Misses 253 253
+ Partials 9 8 -1 ☔ View full report in Codecov by Sentry. |
| @@ -18,7 +18,7 @@ public async Task I_can_view_application_event_logs_when_event_logs_are_enabled( | |||
|
|
|||
| // Act | |||
| var response = await passwordless.GetEventLogAsync( | |||
| new GetEventLogRequest { PageNumber = 1, NumberOfResults = 100 } | |||
| new GetEventLogRequest(1, 100) | |||
There was a problem hiding this comment.
non-blocking-nit: I find this less readable.
There was a problem hiding this comment.
Same. It's possible to keep the same signature with required and init but that involves more boilerplate though. We can still do it if it's important.
|
@jrmccannon curious about your opinion since you added some of these models |
|
A lot of the default values were mainly so that if the classes were newed up, we could avoid some null reference exceptions. Since we're now enforcing the records to be created, you would have to explicitly assign null to the newed up record. |
|
@abergs @jrmccannon do you think this is approvable or would you rather I revert something? |
This reverts commit 30273e1.
Found a way to assign inline docs to properties -- using the
<para>on the type definition.Migrated some of our older models to use
{ get; init; }and/orrecordwhere possible. This is a breaking change to models related to event logging, but those have not been released yet.